Adding swagger changes for Virtual Network Rules#1095
Adding swagger changes for Virtual Network Rules#1095dsgouda merged 7 commits intoAzure:masterfrom dnayantara:swaggerdocbranch
Conversation
|
@dnayantara, |
nathannfan
left a comment
nathannfan
left a comment
There was a problem hiding this comment.
Looks good! A few minor changes for it to work for SDK, but it's close.
| "info": { | ||
| "version": "2015-05-01-preview", | ||
| "title": "SqlManagementClient", | ||
| "description": "The Database management API provides a RESTful set of web APIs that interact with Database services to manage your databases. The API enables users to create, retrieve, update, and delete databases, servers, and other entities." |
There was a problem hiding this comment.
Could you update description to something more specific to VNetRules?
There was a problem hiding this comment.
Sure. Makes sense
| "VnetFirewallRule" | ||
| ], | ||
| "description": "Returns information about a Vnet Firewall Rule.", | ||
| "operationId": "VnetFirewallRule_Get", |
There was a problem hiding this comment.
Sorry, I should clarify. VnetFirewallRules_Get - we've been going plural_operation for operation id
| } | ||
| } | ||
| }, | ||
| "x-ms-long-running-operation": true |
There was a problem hiding this comment.
Do you return 202? If so, I think you'll need to add
"202": {
"description": "Accepted"
}
to responses. No need to include the response body for 202 that in the swagger or examples though, just the response code with an empty body.
There was a problem hiding this comment.
I'm assuming this operation follows azure async pattern, which returns 202 initially?
There was a problem hiding this comment.
Yes it does. I will add that.
| } | ||
| }, | ||
| "definitions": { | ||
| "OperationListResult": { |
There was a problem hiding this comment.
Are the Operations definitions being used?
There was a problem hiding this comment.
I dont think so. I didnt think we would have to do anything separately for the operations
There was a problem hiding this comment.
You're right, we only need them in one file per API version. You can remove all the Operations definitions then.
There was a problem hiding this comment.
This should also clear up one of the validation errors
|
@dnayantara Looks like the validation failures are because of parameter description mismatches. Copying the parameters from https://github.com/Azure/azure-rest-api-specs/blob/master/arm-sql/2015-05-01-preview/swagger/blobAuditingPolicies.json should fix it. |
dsgouda
left a comment
dsgouda
left a comment
There was a problem hiding this comment.
Requesting minor changes
| "Gets the virtual network firewall rule": { "$ref": "../examples/VirtualNetworkRulesGet.json" } | ||
| }, | ||
| "parameters": [ | ||
| { |
There was a problem hiding this comment.
nit: Maintain parameter ordering, place SubscriptionId and ApiVersion together at the beginning or the end of the parameters section
| "Create or update a virtual network firewall rule": { "$ref": "../examples/VirtualNetworkRulesCreateOrUpdate.json" } | ||
| }, | ||
| "parameters": [ | ||
| { |
| "x-ms-examples": { | ||
| "Delete a virtual network firewall rule": { "$ref": "../examples/VirtualNetworkRulesDelete.json" } | ||
| }, | ||
| "parameters": [ |
| "Lists the virtual network firewall rules": { "$ref": "../examples/VirtualNetworkRulesList.json" } | ||
| }, | ||
| "parameters": [ | ||
| { |
| "properties": { | ||
| "$ref": "#/definitions/VnetFirewallRuleResourceProperties" | ||
| }, | ||
| "location": { |
There was a problem hiding this comment.
Looks like a Resource, there are a couple of options here either reference this model or redefine the same structure here and do an allOf. (I'd recommend not redefining simply since its duplication and overhead)
There was a problem hiding this comment.
Yes, please copy Resource+TrackedResource/ProxyResource from other SQL files and use allOf to include TrackedResource/ProxyResource as appropriate.
| "description": "Properties of a vnet firewall rule resource.", | ||
| "type": "object", | ||
| "properties": { | ||
| "virtualNetworkSubnetId": { |
There was a problem hiding this comment.
Should this be readonly? Or do you allow this as a part of the request object?
There was a problem hiding this comment.
This is allowed as part of the request object. I dont think this is readonly
| } | ||
| }, | ||
| "nextLink": { | ||
| "type": "string" |
| "$ref": "#/parameters/ServerNameParameter" | ||
| }, | ||
| { | ||
| "name": "vnetFirewallRuleName", |
There was a problem hiding this comment.
this should be name: parameters
There was a problem hiding this comment.
name:parameters makes sense for a body parameter. This is correct
| "VnetFirewallRule" | ||
| ], | ||
| "description": "Returns information about Vnet Firewall Rules.", | ||
| "operationId": "VnetFirewallRules_List", |
There was a problem hiding this comment.
should be VnetFirewallRules_ListByServer
| "properties": { | ||
| "$ref": "#/definitions/VnetFirewallRuleResourceProperties" | ||
| }, | ||
| "location": { |
There was a problem hiding this comment.
Yes, please copy Resource+TrackedResource/ProxyResource from other SQL files and use allOf to include TrackedResource/ProxyResource as appropriate.
| "type": "object", | ||
| "properties": { | ||
| "properties": { | ||
| "$ref": "#/definitions/VnetFirewallRuleResourceProperties" |
| } | ||
| } | ||
| }, | ||
| "VnetFirewallRuleList": { |
There was a problem hiding this comment.
VnetFirewallRuleListResult
| "$ref": "#/definitions/VnetFirewallRule" | ||
| } | ||
| }, | ||
| "nextLink": { |
There was a problem hiding this comment.
Do you actually have paging? If no, remove this and add x-ms-paging: { nextLinkName: null } to the list operation.
| }, | ||
| "type": { | ||
| "type": "string" | ||
| } |
There was a problem hiding this comment.
Is this a proxy resource? If so, remove location, id, name, and type from this definition and use allOf, e.g. https://github.com/Azure/azure-rest-api-specs/blob/master/arm-sql/2014-04-01/swagger/sql.core.json#L1627
| } | ||
| } | ||
| }, | ||
| "VnetFirewallRuleResourceProperties": { |
There was a problem hiding this comment.
Should be named VnetFirewallRuleProperties
| "x-ms-paging": { | ||
| "nextLinkName": "null" | ||
| }, | ||
| "parameters": [ |
There was a problem hiding this comment.
Formatting looks a little off
| "VnetFirewallRule": { | ||
| "description": "Vnet Firewall Rule Resource.", | ||
| "type": "object", | ||
| "properties": { |
|
FYI the ListByOperationsValidation linter error regarding ListByServer should be ignored. See discussion with @veronicagg at #1005 (comment) |
| } | ||
| ] | ||
| }, | ||
| "VnetFirewallRule": { |
There was a problem hiding this comment.
nit: What I implied was cross-referencing using definitions that already exist under your RP like so
https://github.com/Azure/azure-rest-api-specs/blob/master/arm-recoveryservices/2016-06-01/swagger/replicationusages.json#L46
This is not a requirement but reduces duplication
There was a problem hiding this comment.
Not making this change as @jaredmoo said in the email, cross references don't work.
There was a problem hiding this comment.
Replied on the email thread. However, won't block on this.
| "in": "path", | ||
| "description": "The name of the Virtual Network Firewall Rule.", | ||
| "required": true, | ||
| "type": "string" |
There was a problem hiding this comment.
needs "x-ms-parameter-location": "method"
| "tags": [ | ||
| "VnetFirewallRule" | ||
| ], | ||
| "description": "Returns information about a Vnet Firewall Rule.", |
There was a problem hiding this comment.
nit: can you use "virtual network firewall rule" (no capitalization) instead of "Vnet Firewall Rule" throughout this file? That will make Azure CLI commands look better when you get to it.
There was a problem hiding this comment.
Ill change it to virtual network rules since thats what we are calling it to the customers.
| "tags": [ | ||
| "VnetFirewallRule" | ||
| ], | ||
| "description": "Creates a new Vnet Firewall Rule or updates an existing Vnet Firewall Rule.", |
There was a problem hiding this comment.
Creates or updates a virtual network firewall rule
| "properties": { | ||
| "properties": { | ||
| "$ref": "#/definitions/VnetFirewallRuleProperties", | ||
| "x-ms-flatten": "true" |
There was a problem hiding this comment.
This should be x-ms-client-flatten
| "x-ms-examples": { | ||
| "Lists the virtual network firewall rules": { "$ref": "../examples/VirtualNetworkRulesList.json" } | ||
| }, | ||
| "x-ms-paging": { |
There was a problem hiding this comment.
This should be x-ms-pageable, please recheck extension names
|
@azuresdkci Test this please |
|
I will add another iteration which has been directly generated from the source code soon. |
| "type": "object", | ||
| "properties": { | ||
| "nextLink": { | ||
| "description": "Link to retrieve next page of results.", |
There was a problem hiding this comment.
Please mark this as readonly
| "$ref": "#/parameters/ServerNameParameter" | ||
| }, | ||
| { | ||
| "name": "vnetFirewallRuleName", |
There was a problem hiding this comment.
name:parameters makes sense for a body parameter. This is correct
| "in": "body", | ||
| "description": "The requested vnetFirewall Resource state.", | ||
| "required": true, | ||
| "schema": { |
There was a problem hiding this comment.
nit: Just want to point out that the VnetFirewallRule model has no required properties which means it could theoretically be an empty object, please verify that the service is fine with this.
There was a problem hiding this comment.
No. the parameters should be required. I will change this.
| @@ -250,6 +250,7 @@ | |||
| "x-ms-azure-resource": true | |||
| }, | |||
| "ProxyResource": { | |||
There was a problem hiding this comment.
Did you plan to add required properties for VnetFirewallRule in this commit?
There was a problem hiding this comment.
Lets just leave it like this for now. I may make changes later.
There was a problem hiding this comment.
I'm fine with it as long as the service is OK with an empty body parameter
There was a problem hiding this comment.
I am with the service team and we are ok with this for now. Please accept pull request if everything else looks good.
|
No modification for NodeJS |
|
No modification for Ruby |
10 participants
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-versionin the path should match theapi-versionin the spec).Quality of Swagger